Skip to content

Deprecate override in favour of triage #2005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
May 23, 2025
Merged

Deprecate override in favour of triage #2005

merged 10 commits into from
May 23, 2025

Conversation

marschattha
Copy link
Member

@Copilot Copilot AI review requested due to automatic review settings May 1, 2025 19:49
Copilot
Copilot AI reviewed May 1, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR deprecates the legacy override functionality in favor of a new triage configuration for issue transformation. Key changes include the introduction of the triage module with new configuration structures, removal of unused override logic, and updates to the planner to build triages from both triage and legacy override settings.

Reviewed Changes

Copilot reviewed 7 out of 13 changed files in this pull request and generated no comments.

Show a summary per file
File Description
qlty-config/src/config/triage.rs Introduces new triage configuration and implementation.
qlty-config/src/config/overrides.rs Deprecates override functionality by removing redundant code.
qlty-config/src/config.rs Re-exports the new triage components and updates module order.
qlty-cli/tests/cmd/check/triage.toml Adds a test configuration for triage.
qlty-cli/tests/cmd/check/triage.in/.qlty/qlty.toml Updates TOML config to include triage settings.
qlty-cli/tests/cmd/check/override.in/.qlty/qlty.toml Retains a legacy override test configuration with a deprecation notice.
qlty-check/src/planner.rs Modifies the plan builder to prefer triage over override.
Files not reviewed (6)
  • qlty-cli/tests/cmd/check/override.stderr: Language not supported
  • qlty-cli/tests/cmd/check/triage.in/.gitignore: Language not supported
  • qlty-cli/tests/cmd/check/triage.in/sample.sh: Language not supported
  • qlty-cli/tests/cmd/check/triage.in/sample2.sh: Language not supported
  • qlty-cli/tests/cmd/check/triage.stderr: Language not supported
  • qlty-cli/tests/cmd/check/triage.stdout: Language not supported
Comments suppressed due to low confidence (2)

qlty-cli/tests/cmd/check/triage.in/.qlty/qlty.toml:17

  • The key 'match.plugin' does not match the expected field name 'plugins' in the Triage configuration. Consider updating this key to 'plugins' to ensure proper deserialization.
match.plugin = "exists"

qlty-cli/tests/cmd/check/triage.in/.qlty/qlty.toml:18

  • The key 'match.rule' does not match the expected field name 'rules' in the Triage configuration. Please update this key to 'rules' for consistency.
match.rule = "fail"

@qltysh qltysh
Copy link
Contributor

qltysh bot commented May 1, 2025 •
edited
Loading

Diff Coverage for macos-15: The code coverage on the diff in this pull request is 88.3%.

Total Coverage for macos-15: This PR will increase coverage by 0.18%.

File Coverage Changes
Path File Coverage Δ Indirect
qlty-check/src/executor/driver.rs -1.4 √
qlty-check/src/parser/terraform.rs 96.4 √
qlty-check/src/planner.rs 0.8
qlty-check/src/planner/driver.rs 7.4 √
qlty-check/src/planner/invocation_directory.rs -8.1 √
qlty-check/src/planner/target_batcher.rs -6.8 √
qlty-config/src/config/plugin.rs 10.3 √
qlty-config/src/config/triage.rs 83.8
qlty-config/src/library.rs -0.5 √
qlty-config/src/sources/git_source.rs -1.3 √
qlty-types/src/lib.rs 8.4 √
🛟 Help

  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

@qltysh qltysh
Copy link
Contributor

qltysh bot commented May 1, 2025 •
edited
Loading

Diff Coverage for ubuntu-latest: The code coverage on the diff in this pull request is 88.3%.

Total Coverage for ubuntu-latest: This PR will increase coverage by 0.19%.

File Coverage Changes
Path File Coverage Δ Indirect
qlty-check/src/executor/driver.rs -0.1 √
qlty-check/src/parser/terraform.rs 96.4 √
qlty-check/src/planner.rs 0.8
qlty-check/src/planner/driver.rs 0.1 √
qlty-check/src/planner/invocation_directory.rs 0.2 √
qlty-check/src/planner/target_batcher.rs 0.0 √
qlty-cli/src/auth/auth_flow.rs -0.5 √
qlty-config/src/config/plugin.rs -0.2 √
qlty-config/src/config/triage.rs 83.8
qlty-config/src/library.rs -0.5 √
qlty-config/src/sources/git_source.rs 0.5 √
qlty-types/src/lib.rs 4.7 √
🛟 Help

  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

@qltysh qltysh
Copy link
Contributor

qltysh bot commented May 1, 2025 •
edited
Loading

❌ 1 blocking issue (1 total)

Tool Category Rule Count
ripgrep Lint // TODO: Issues without a path are not filterable 1 ❌

qltysh[bot]
qltysh bot reviewed May 1, 2025
View reviewed changes
qlty-config/src/config/triage.rs
if let Some(path) = issue.path() {
glob_set.as_ref().unwrap().is_match(path)
} else {
// TODO: Issues without a path are not filterable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// TODO: Issues without a path are not filterable [ripgrep:TODO]

@marschattha marschattha requested a review from brynary May 1, 2025 21:09
@marschattha marschattha mentioned this pull request May 13, 2025
Merged
brynary
brynary reviewed May 20, 2025
View reviewed changes
qlty-config/src/config/triage.rs
pub category: Option<String>,

#[serde(default)]
pub mode: Option<IssueMode>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think disabled is allowed as an IssueMode but should be rejected here. So I guess what we currently call "IssueMode" is really a plugin mode and an Issue Mode is a plugin mode except "disabled"

qlty-config/src/config/triage.rs Outdated
pub level: Option<String>,

#[serde(default)]
pub category: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be an enum instead a string?

qlty-config/src/config/triage.rs Outdated
#[derive(Debug, Serialize, Deserialize, Default, Clone, JsonSchema)]
pub struct Set {
#[serde(default)]
pub level: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be an enum instead a string?

qlty-config/src/config/triage.rs Outdated
pub struct Triage {
#[serde(default)]
#[serde(rename = "match")]
pub _match: Match,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do r#match as an alternative to _match

@marschattha marschattha force-pushed the ma/replace_override_with_triage branch from 1375b77 to d1f17c4 Compare May 20, 2025 21:03
qltysh[bot]
qltysh bot reviewed May 20, 2025
View reviewed changes
@marschattha marschattha force-pushed the ma/replace_override_with_triage branch from d1f17c4 to f3e1818 Compare May 20, 2025 21:08
brynary
brynary reviewed May 21, 2025
View reviewed changes
brynary
brynary reviewed May 22, 2025
View reviewed changes
qlty-types/src/lib.rs Outdated
@@ -383,3 +387,65 @@ impl AddAssign for tests::v1::CoverageSummary {
*self = *self + other;
}
}

impl JsonSchema for analysis::v1::Level {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of manually implementing the JsonSchema trait, I think it would be better if we could have the Prost protobuf generator include a JsonSchema in the derive macro during codegen.

It looks like this is possible: tokio-rs/prost#1064

https://docs.rs/prost-build/latest/prost_build/struct.Config.html#method.type_attribute

However, before going down this rabbit hole, what I would recommend is that you manually edit the generated files to add JsonSchema to the existing derive statements, so:

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)]
#[repr(i32)]
pub enum Level {
    Unspecified = 0,
    Note = 10,
    Fmt = 20,
    Low = 30,
    Medium = 40,
    High = 50,
}

Becomes:

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration, JsonSchema)]
#[repr(i32)]
pub enum Level {
    Unspecified = 0,
    Note = 10,
    Fmt = 20,
    Low = 30,
    Medium = 40,
    High = 50,
}

If the manual edit works, then we just need to get Prost to output the code the way we want.

Copy link
Member Author

@marschattha marschattha May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was concerned that manually modifying generated file will cause long term friction, every time the files are generated as JsonSchema would get removed and then someone would have to figure out the compile error put it back in place.

Even right now every time I make a change with protos I have to be careful to pick the changes intended and commit those, as it makes a lot of changes to the generated files every time I run the command.

@brynary
Copy link
Member

brynary commented May 22, 2025

@claude review this PR

@claude Claude
Copy link

claude bot commented May 22, 2025 •
edited
Loading

Claude finished @brynary's task —— View job

I'll review this PR in detail. Here's my plan:

  • Read repository-specific instructions
  • Review changed files to understand the PR
  • Analyze PR description and comments
  • Examine existing review comments
  • Check for code quality issues and potential improvements
  • Provide comprehensive PR review feedback

PR Review: Deprecate override in favour of triage

This PR replaces the existing override functionality with a new triage system. Overall, the implementation is solid and improves the codebase's design. Here's my comprehensive review:

Positive Aspects

  • The new triage API provides a cleaner, more structured approach than the previous override system
  • Good backward compatibility by supporting the deprecated override field in configuration
  • Use of strong typing with enums for Level and Category matches Rust best practices
  • Well-organized test cases that verify the functionality works as expected
  • Implementation of JsonSchema for the enums enables proper schema generation

Issues and Suggestions

  1. In triage.rs:

    • Line 125: The TODO comment about "Issues without a path are not filterable" should be addressed before merging. Consider returning true instead of false to avoid filtering out issues without paths, or add a config flag to control this behavior.
    • Consider validating that IssueMode::Disabled is not used in the Set struct, as mentioned in brynary's comment. A plugin mode and issue mode seem to be conceptually different.

  2. In planner.rs:

    • Line 299: Fix the Clippy warning by directly using mode: issue_override.mode instead of mode: issue_override.mode.clone() since IssueMode implements Copy.

  3. In lib.rs:

    • Consider the suggestion to use derive macros for JsonSchema in the generated files instead of manual implementation. This would simplify the code and reduce maintenance overhead.
    • If modifying the generated files isn't feasible right now, document this design decision for future reference.

  4. General Improvements:

    • Consider implementing the From trait to transform between different enum types, as suggested by brynary. This would make the code more idiomatic and cleaner.
    • The Set struct in triage.rs could have a method to validate that only appropriate IssueMode values are used.
    • Rename Match to something more descriptive like TriageMatcher or IssueSelector to avoid confusion with the Rust keyword (even though using r#match works).

  5. Documentation:

    • The PR would benefit from documentation explaining the triage feature, especially in the form of comments on public structs and methods.
    • Consider updating user-facing documentation to reflect this change and to guide users in migrating from override to triage.

These improvements would further enhance the quality of this well-structured PR.

@marschattha marschattha requested a review from brynary May 22, 2025 21:34
@brynary brynary merged commit 406ff0a into main May 23, 2025
14 checks passed
@brynary brynary deleted the ma/replace_override_with_triage branch May 23, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qltysh qltysh[bot] qltysh[bot] left review comments

Copilot code review Copilot Copilot left review comments

@brynary brynary brynary approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@marschattha @brynary